Skip to content

added part count validation for download request#6353

Merged
Fred1155 merged 18 commits intomasterfrom
bole/partcount-range-validation
Sep 12, 2025
Merged

added part count validation for download request#6353
Fred1155 merged 18 commits intomasterfrom
bole/partcount-range-validation

Conversation

@Fred1155
Copy link
Copy Markdown
Contributor

@Fred1155 Fred1155 commented Aug 18, 2025 •
edited
Loading

Motivation and Context

For the part get request for S3 transfer manager, When a response is received, the S3 Transfer Manager MUST check the value of ContentRange and validate that it matches with the expected range for the specific part number. After all requests have been sent, we should validate that the total number of part GET requests sent matches with the expected PartsCount. This PR implements the validation.

Modifications

Added validation method in MultipartDownloaderSubscriber class

Testing

Added wiremock test for failed test

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@Fred1155 Fred1155 requested a review from a team as a code owner August 18, 2025 17:42
zoewangg
zoewangg requested changes Aug 19, 2025
View reviewed changes
Comment thread ...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java Outdated
Comment thread ...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java Outdated
Comment thread ...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java Outdated
Comment thread .changes/next-release/bugfix-AWSSDKforJavav2-17f90b1.json Outdated
Comment thread .changes/next-release/bugfix-AWSSDKforJavav2-17f90b1.json Outdated
Comment thread ...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java Outdated
@Fred1155 Fred1155 requested a review from zoewangg August 21, 2025 21:47
zoewangg
zoewangg reviewed Aug 22, 2025
View reviewed changes
Comment thread ...st/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java Outdated
Comment thread ...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java Outdated
@zoewangg zoewangg requested review from zoewangg and removed request for zoewangg August 22, 2025 23:38
L-Applin
L-Applin suggested changes Aug 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@L-Applin L-Applin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some tests cases for this new validation?

@Fred1155 Fred1155 requested review from L-Applin and millems August 27, 2025 18:22
@Fred1155 Fred1155 changed the title added part count and content range validation for download request added part count validation for download request Aug 28, 2025
@L-Applin
Copy link
Copy Markdown
Contributor

L-Applin commented Aug 29, 2025 •
edited
Loading

I think we need a test with an actual multipart s3AsyncClient, ie:

S3AsyncClient client = S3AsyncClient client = S3AsyncClient
    .builder()
    .multipartEnabled(true)
    // other configs
    .build();

We can still mock the http client but that might be a hard test to mock, let me think about it to see if there is a way to do it.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Sep 11, 2025

Please retry analysis of this Pull-Request directly on SonarQube Cloud

@Fred1155 Fred1155 requested a review from zoewangg September 12, 2025 16:25
zoewangg
zoewangg approved these changes Sep 12, 2025
View reviewed changes
Comment thread .../software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java
aResponse()
.withHeader("x-amz-mp-parts-count", totalPart + "")
.withHeader("ETag", eTag)
.withHeader("Content-Length", String.valueOf(body.length))
Copy link
Copy Markdown
Contributor

@zoewangg zoewangg Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why are we making this change?

Copy link
Copy Markdown
Contributor Author

@Fred1155 Fred1155 Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this change to do some testing, but that testing is deleted. Figured this change makes the response more "real" so I kept it

@Fred1155 Fred1155 added this pull request to the merge queue Sep 12, 2025
Merged via the queue into master with commit 6350ed6 Sep 12, 2025
38 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 12, 2025

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2025
@Fred1155 Fred1155 deleted the bole/partcount-range-validation branch September 12, 2025 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zoewangg zoewangg zoewangg approved these changes

@millems millems Awaiting requested review from millems

+1 more reviewer

@L-Applin L-Applin L-Applin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Fred1155 @L-Applin @millems @zoewangg